-
-
Notifications
You must be signed in to change notification settings - Fork 952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved the Terminal Watchfaces UI #2135
base: main
Are you sure you want to change the base?
Improved the Terminal Watchfaces UI #2135
Conversation
Build size and comparison to main:
|
+ Reorder code to match the widgets order in the UI. + Use InfintimeTheme Colors instead of hardcoded hex values + Added a new InfinitimeTheme color: gray, using it to turn certain values gray when they contain no data + Implement @vkareh's [variable battery icon](InfiniTimeOrg#1964) color to the battery percentage text. + Replaced the 'You have mail.' notification message with the message '[1]+ Notify' to better fit the terminal lore.
c312a6c
to
d2382e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use terminal so no strong opinion on visual style but looks nice to me
Maybe we should use this opportunity to fix the badly named fields (they should be camelCase)? But that could also be another PR, definitely not a blocker
// between red and green, thus avoiding the darker RGB on medium battery | ||
// charges and giving us a much nicer color range. | ||
uint8_t hue = batteryPercentRemaining.Get() * 120 / 100; | ||
lv_obj_set_style_local_text_color(batteryValue, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hsv_to_rgb(hue, 100, 100)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think colours here are nice, but this shows yellow at 50% right? I think it would make more sense to follow a curve closer to the current battery icon (yellow at 15, red at 5). Not sure what function would represent this well but coming up with something in Desmos/similar would be easy
Also if this is to be used in other places as well, it should probably be moved out so other screens can use it. I don't know if that's the goal here though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is from #1964 , so I think it definitely makes sense to move the colour calculation algorithm to somewhere where other apps can use it
heartbeat = heartRateController.HeartRate(); | ||
heartbeatRunning = heartRateController.State() != Controllers::HeartRateController::States::Stopped; | ||
if (heartbeat.IsUpdated() || heartbeatRunning.IsUpdated()) { | ||
if (heartbeatRunning.Get()) { | ||
lv_label_set_text_fmt(heartbeatValue, "[L_HR]#ee3311 %d bpm#", heartbeat.Get()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline
These changes were originally proposed in PR #2001 along with the addition of weather data on it's own line, but it was suggested they be seperated. This PR is just the UI improvements without the weather, and #2001 will add the weather.
Reorder code to match the widgets order in the UI.
Use InfintimeTheme Colors instead of hardcoded hex values
Added a new InfinitimeTheme color: gray, using it to turn certain values gray when they contain no data
Implement @vkareh's variable battery icon color to the battery percentage text.
Replaced the 'You have mail.' notification message with the message '[1]+ Notify' to better fit the terminal lore.